[shared_preferences] update List<String> encode/decode #8335

tarrinneal · 2024-12-20T03:02:04Z

Follow up to #8187

Updates Android List<String> encoding/decoding to use JSON to avoid potential future breakages from restricted list types.

…code

tarrinneal · 2025-01-03T12:26:14Z

@stuartmorgan ready for review here as well

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

reidbaker · 2025-01-06T16:16:50Z

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

  private @NonNull Map<String, Object> getAllPrefs(
      @NonNull String prefix, @Nullable Set<String> allowList) throws RuntimeException {
    Map<String, ?> allPrefs = preferences.getAll();
    Map<String, Object> filteredPrefs = new HashMap<>();
    for (String key : allPrefs.keySet()) {
      if (key.startsWith(prefix) && (allowList == null || allowList.contains(key))) {
-        filteredPrefs.put(key, transformPref(key, allPrefs.get(key)));
+        filteredPrefs.put(key, transformPref(key, Objects.requireNonNull(allPrefs.get(key))));


What was requireNonNull needed here? Also this code is getting a bit difficult to parse. (A loop with a conditional that sets some transformed filtered data) Consider breaking parts out into methods or adding comments.

It's a change suggested by a warning I was getting in Android Studio. Since the method being called requires nonnull arguments and getting a value from a map doesn't programmatically guarantee a value. In this context, there would always be a value though.

reidbaker · 2025-01-06T16:22:40Z

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

@@ -148,32 +141,14 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin
  private Object transformPref(@NonNull String key, @NonNull Object value) {
    if (value instanceof String) {
      String stringValue = (String) value;
-      if (stringValue.startsWith(LIST_IDENTIFIER)) {
+      if (stringValue.startsWith(JSON_LIST_IDENTIFIER)) {


I think this is where having the keys be similar starts to be important. That said from reading the code the reader does not know the keys are overlapping and it would seem safe in code review to swap the order of these if statements.

Consider making the keys independent so that you are not relying on the contents of the keys. If that is too much code change then consider making a "LIST_PREFIX" that can be checked so that it is at least clear in code what the relationship is.

If you want further reading see
https://www.16elt.com/2024/09/25/first-book-of-byte-sized-tech/

I think the first example explains the principal I am applying.

Consider making the keys independent so that you are not relying on the contents of the keys.

This is something Tarrin and I had discussed offline; making it a prefix avoids this being a potentially (very unlikely, but still possible) breaking change, by using our already-reserved space.

I would strongly prefer we keep that, and adjust the code to make it explicit to readers.

Ok I can understand that fear and why it might fail. Thank you for being open to structuring the code differently so that the overlap is easier to follow for future maintainers.

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

...preferences/shared_preferences_android/example/integration_test/shared_preferences_test.dart

reidbaker · 2025-01-06T16:33:11Z

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 3.0.0


Are the breaking changes required? if not consider fixing/mitigating the security issue independent of the breaking changes. It makes it more likely for them to be adopted.

Breaking changes to shared_preferences have the potential to be quite disruptive to the ecosystem; I would strongly prefer we keep the legacy type support around to making a breaking change.

Fair enough. I originally expected this change was going to end up being "technically" breaking due to the new prefix (before we discussed the prefix extension plan) so I wanted to wrap these long awaited breaking changes in as well. I'll revert them.

Breaking changes to shared_preferences have the potential to be quite disruptive to the ecosystem; I would strongly prefer we keep the legacy type support around to making a breaking change.

Should I revert the error handling changes as well?

Should I revert the error handling changes as well?

I did this.

packages/shared_preferences/shared_preferences_android/lib/src/strings.dart

packages/shared_preferences/shared_preferences_android/lib/src/shared_preferences_android.dart

reidbaker · 2025-01-06T16:37:09Z

packages/shared_preferences/shared_preferences_android/lib/src/shared_preferences_android.dart

@@ -39,20 +41,27 @@ class SharedPreferencesAndroid extends SharedPreferencesStorePlatform {
  Future<bool> setValue(String valueType, String key, Object value) async {
    switch (valueType) {
      case 'String':
-        return _api.setString(key, value as String);
+        value as String;
+        if (value.startsWith(listPrefix) ||


Will this list grow? if so consider making a set of prefixes to check.

We don't generally add types to shared_preferences since anything that all platforms don't support ends up with unpleasant complexity like the List<String> mess we are currently dealing with.

stuartmorgan · 2025-01-06T17:49:26Z

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 3.0.0


Breaking changes to shared_preferences have the potential to be quite disruptive to the ecosystem; I would strongly prefer we keep the legacy type support around to making a breaking change.

...roid/android/src/main/kotlin/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.kt

stuartmorgan · 2025-01-06T17:55:34Z

packages/shared_preferences/shared_preferences_android/lib/src/shared_preferences_android.dart

@@ -39,20 +41,27 @@ class SharedPreferencesAndroid extends SharedPreferencesStorePlatform {
  Future<bool> setValue(String valueType, String key, Object value) async {
    switch (valueType) {
      case 'String':
-        return _api.setString(key, value as String);
+        value as String;
+        if (value.startsWith(listPrefix) ||


We don't generally add types to shared_preferences since anything that all platforms don't support ends up with unpleasant complexity like the List<String> mess we are currently dealing with.

.../shared_preferences/shared_preferences_android/lib/src/shared_preferences_async_android.dart

…code

update List<String> encode/decode and other simple breaking changes

e772224

tarrinneal requested a review from stuartmorgan December 20, 2024 03:02

github-actions bot added platform-android platform-ios platform-macos p: shared_preferences labels Dec 20, 2024

tarrinneal added 3 commits January 2, 2025 11:36

fix test set up and legacy android system

089d11d

Merge branch 'main' of github.com:flutter/packages into listString-en…

29f3662

…code

format analyze update to latest pigeon

c0c08df

tarrinneal marked this pull request as ready for review January 3, 2025 12:25

tarrinneal requested a review from reidbaker as a code owner January 3, 2025 12:25

remove unneeded braces from switch statement

2ef0a01

reidbaker reviewed Jan 6, 2025

View reviewed changes

stuartmorgan reviewed Jan 6, 2025

View reviewed changes

tarrinneal added 2 commits January 6, 2025 14:39

revert all breaking changes

bad5268

Merge branch 'main' of github.com:flutter/packages into listString-en…

7a013b1

…code

github-actions bot removed platform-ios platform-macos labels Jan 6, 2025

tarrinneal added 4 commits January 6, 2025 15:23

rest of the comments

2c10fb7

update test getter to match setter

614cbe4

fix tests that I broke fixing tests

e622f04

fix android code for error handling changes reversion

d317354

tarrinneal requested a review from stuartmorgan January 7, 2025 17:02

tarrinneal changed the title ~~[shared_preferences] update List<String> encode/decode and other simple breaking changes~~ [shared_preferences] update List<String> encode/decode Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[shared_preferences] update List<String> encode/decode #8335

[shared_preferences] update List<String> encode/decode #8335

tarrinneal commented Dec 20, 2024 •

edited

Loading

tarrinneal commented Jan 3, 2025

reidbaker Jan 6, 2025

tarrinneal Jan 6, 2025

reidbaker Jan 6, 2025

stuartmorgan Jan 6, 2025 •

edited

Loading

reidbaker Jan 6, 2025

reidbaker Jan 6, 2025

stuartmorgan Jan 6, 2025

tarrinneal Jan 6, 2025

tarrinneal Jan 6, 2025

tarrinneal Jan 6, 2025

reidbaker Jan 6, 2025

stuartmorgan Jan 6, 2025

stuartmorgan Jan 6, 2025

stuartmorgan Jan 6, 2025

[shared_preferences] update List<String> encode/decode #8335

Are you sure you want to change the base?

[shared_preferences] update List<String> encode/decode #8335

Conversation

tarrinneal commented Dec 20, 2024 • edited Loading

tarrinneal commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal commented Dec 20, 2024 •

edited

Loading

stuartmorgan Jan 6, 2025 •

edited

Loading